Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement LLVM-based Lexer for IR #742

Merged
merged 18 commits into from
Aug 8, 2022

Conversation

fivosts
Copy link
Contributor

@fivosts fivosts commented Aug 2, 2022

Adds 2 observations:

  • Lexedir
  • LexedirTuple

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 2, 2022
@fivosts
Copy link
Contributor Author

fivosts commented Aug 2, 2022

@hughleat

@fivosts
Copy link
Contributor Author

fivosts commented Aug 2, 2022

Not sure why 7 commits show up in the PR instead of just 2, probably because I branched from stable.

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2022

Codecov Report

Merging #742 (48c0ec5) into development (b58e83b) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #742      +/-   ##
===============================================
- Coverage        88.75%   88.73%   -0.03%     
===============================================
  Files              129      130       +1     
  Lines             7855     7864       +9     
===============================================
+ Hits              6972     6978       +6     
- Misses             883      886       +3     
Impacted Files Coverage Δ
compiler_gym/envs/llvm/lexed_ir.py 100.00% <100.00%> (ø)
compiler_gym/envs/llvm/llvm_env.py 91.66% <100.00%> (+0.04%) ⬆️
compiler_gym/envs/llvm/specs.py 100.00% <100.00%> (ø)
compiler_gym/service/service_cache.py 89.47% <0.00%> (-2.64%) ⬇️
compiler_gym/service/connection.py 75.91% <0.00%> (-1.68%) ⬇️
...loop_tool/service/loop_tool_compilation_session.py 89.86% <0.00%> (+2.02%) ⬆️

Copy link
Contributor

@ChrisCummins ChrisCummins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks @fivosts! I've left some very nitpicky comments inline. Few small things:

  • rebase on development.
  • you should run the pre-commit hooks here which will sadly obliterate all your nice vertical aligned code 🙂
  • I think we need a copy of the LLVM License file at compiler_gym/third_party/LexedIR/LICENSE.txt. You can copy the one from compiler_gym/third_party/llvm/LICENSE.txt.
  • I don't see any changes to the tests, so development/tests/llvm/observation_spaces_test.py should break. Could you add a test to cover the new space? Take a look here for an example

Cheers,
Chris

compiler_gym/envs/llvm/BUILD Outdated Show resolved Hide resolved
compiler_gym/envs/llvm/CMakeLists.txt Outdated Show resolved Hide resolved
compiler_gym/envs/llvm/service/BUILD Outdated Show resolved Hide resolved
compiler_gym/envs/llvm/service/Observation.cc Outdated Show resolved Hide resolved
compiler_gym/envs/llvm/specs.py Outdated Show resolved Hide resolved
compiler_gym/third_party/LexedIR/BUILD Outdated Show resolved Hide resolved
compiler_gym/third_party/LexedIR/decoy_main.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@ChrisCummins ChrisCummins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my nitpicks. Here's a couple more 🙂

Something looks fishy about the git history here (130 commits!), may need rebasing/cherry-picking

Cheers,
Chris

@@ -0,0 +1 @@
/private/home/foivos/CompilerGym/cmake_build/compiler_gym/third_party/autophase/compute_autophase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was checked in by accident

@@ -0,0 +1 @@
/private/home/foivos/CompilerGym/cmake_build/compiler_gym/third_party/programl/compute_programl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

tests/llvm/observation_spaces_test.py Outdated Show resolved Hide resolved
@ChrisCummins
Copy link
Contributor

ChrisCummins commented Aug 4, 2022

Note the "Pre-commit" job is failing with a few BUILD file errors. You could run buildifier locally or just copy the patch from the CI output

@fivosts fivosts force-pushed the lexed_IR branch 2 times, most recently from ab3a7c9 to 820abdc Compare August 5, 2022 21:14
@fivosts
Copy link
Contributor Author

fivosts commented Aug 5, 2022

Rebasing to dev probably did this. Did fresh rebase, removed broken symlinks, ran buildifier and cherry picked my commits, should be fine now. Let me know if there's anything else I have missed

@ChrisCummins
Copy link
Contributor

LGTM. The CI test failures are known problems and not caused by this change. Merging, thanks @fivosts!

Cheers,
Chris

@ChrisCummins ChrisCummins merged commit 8d25c10 into facebookresearch:development Aug 8, 2022
This was referenced Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants